WIP: [Storage] Windows tests should all use vTPM - module CDI Clone#4571
WIP: [Storage] Windows tests should all use vTPM - module CDI Clone#4571acinko-rh wants to merge 5 commits intoRedHatQE:mainfrom
Conversation
Co-Authored: Claude Code Signed-off-by: Adam Cinko <acinko@redhat.com>
|
/wip |
|
Important Review skippedIgnore keyword(s) in the title. ⛔ Ignored keywords (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds Windows vTPM provisioning and validation: a helper that creates Windows VMs with vTPM from cloned DataVolumes, validates guest-agent OS info, and conditionally verifies vTPM with admin privileges. Adds a tier3 test cloning a Windows 11 DV and a utility to verify vTPM at hypervisor and guest levels. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
acinko-rh
changed the title
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot
requested review from
josemacassan and
kshvaika
openshift-virtualization-qe-bot
requested review from
Ahmad-Hafe,
jpeimer and
kgoldbla
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/storage/utils.py (1)
541-584:⚠️ Potential issue | 🟠 Major[HIGH] Landing a helper already labeled "DEBUG ONLY - MARKED FOR DELETION" contradicts the dead-code policy.
The docstring and the trailing
# TODO: Remove - debug onlyon Line 541 announce this function is scaffolding, yet it's imported and exercised bytest_successful_vm_from_cloned_dv_windows_with_vtpmintests/storage/cdi_clone/test_clone.py. Either the verification is part of the intended coverage (then drop the "debug only" labeling and keep it properly) or it isn't (then don't merge it). Shipping code pre-flagged for deletion is how dead code quietly permanents itself in the repo.As per coding guidelines: "No dead code in Python. Every function, variable, fixture MUST be used or removed."
✂️ Suggested cleanup
-def verify_vtpm_in_windows_vm(vm, admin_client): # TODO: Remove - debug only +def verify_vtpm_in_windows_vm(vm, admin_client): """ Verify vTPM is properly configured and detected in a Windows VM. - **DEBUG ONLY - MARKED FOR DELETION** - Performs two-layer validation:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/storage/utils.py` around lines 541 - 584, The helper verify_vtpm_in_windows_vm is labelled "DEBUG ONLY - MARKED FOR DELETION" yet is referenced by test_successful_vm_from_cloned_dv_windows_with_vtpm, so remove the dead-code marker and make the function a first-class test utility: delete the "# TODO: Remove - debug only" comment, update the docstring to reflect it is an intentional test helper (remove "DEBUG ONLY"), and ensure any module exports/imports remain correct so tests continue to import verify_vtpm_in_windows_vm; alternatively, if you truly intend to delete it, remove all imports/usages (notably in tests/storage/cdi_clone/test_clone.py) and delete the function instead.tests/storage/cdi_clone/test_clone.py (1)
1-3:⚠️ Potential issue | 🟡 Minor[MEDIUM] New feature test requires an STP (or RFE/Jira epic) link in the module docstring.
The module docstring is still just
"""Clone tests""", but this PR adds a new feature test (test_successful_vm_from_cloned_dv_windows_with_vtpm) covering vTPM on cloned Windows DVs. As per coding guidelines: "Every new feature test module MUST include an STP link in the module docstring. If there is no STP, the module docstring MUST include a link to the RFE or Jira epic (not support cases) for coverage tracking." The PR description also has an empty Jira ticket field, which needs to be filled in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/storage/cdi_clone/test_clone.py` around lines 1 - 3, The module docstring currently just says "Clone tests" but this PR adds the new feature test test_successful_vm_from_cloned_dv_windows_with_vtpm, so update the module docstring to include the required STP link (or, if none, the RFE/Jira epic URL) for coverage tracking and also populate the empty Jira ticket field in the PR description; modify the top-of-file docstring in the test module to include a one-line reference (e.g., "STP: <link>" or "RFE/Jira: <epic-link>") alongside the existing summary so automated checks and reviewers can verify the traceability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/storage/cdi_clone/test_clone.py`:
- Line 248: The test contains a placeholder Polarion ID in the pytest marker:
replace pytest.mark.polarion("CNV-XXXXX") in
tests/storage/cdi_clone/test_clone.py with the real Polarion/Jira ID for this
test (use the official ticket string), and update the PR description’s empty
Jira ticket field to the same ID so reporting and traceability are correct;
ensure the marker value is a non-placeholder string and re-run the
test/reporting to verify the ID appears properly.
- Around line 72-110: Move the helper
create_windows_vm_with_vtpm_validate_guest_agent_info out of the test module
into tests/storage/utils.py (next to
create_windows_vm_validate_guest_agent_info), make admin_client a required
positional argument (remove the =None default) so vTPM validation always runs,
relocate the imports to the top of the new module (import
validate_os_info_vmi_vs_windows_os from utilities.ssp, wait_for_windows_vm from
utilities.virt, and verify_vtpm_in_windows_vm from tests.storage.utils or the
correct canonical module), and remove the runtime None check (do not gate
verify_vtpm_in_windows_vm with if admin_client; call it directly). Ensure any
remaining None comparisons use identity (if x is None) when applicable.
- Line 103: The test creates the VM in a halted state
(VirtualMachine.RunStrategy.HALTED) so wait_for_windows_vm(vm=vm, ...) fails
because it immediately runs SSH; call running_vm(vm=vm) to transition the VM to
Running before calling wait_for_windows_vm. Insert a running_vm(vm=vm)
invocation inside the context manager just prior to wait_for_windows_vm
(mirroring create_windows19_vm) so the VM is started and SSH connectivity is
available.
---
Outside diff comments:
In `@tests/storage/cdi_clone/test_clone.py`:
- Around line 1-3: The module docstring currently just says "Clone tests" but
this PR adds the new feature test
test_successful_vm_from_cloned_dv_windows_with_vtpm, so update the module
docstring to include the required STP link (or, if none, the RFE/Jira epic URL)
for coverage tracking and also populate the empty Jira ticket field in the PR
description; modify the top-of-file docstring in the test module to include a
one-line reference (e.g., "STP: <link>" or "RFE/Jira: <epic-link>") alongside
the existing summary so automated checks and reviewers can verify the
traceability.
In `@tests/storage/utils.py`:
- Around line 541-584: The helper verify_vtpm_in_windows_vm is labelled "DEBUG
ONLY - MARKED FOR DELETION" yet is referenced by
test_successful_vm_from_cloned_dv_windows_with_vtpm, so remove the dead-code
marker and make the function a first-class test utility: delete the "# TODO:
Remove - debug only" comment, update the docstring to reflect it is an
intentional test helper (remove "DEBUG ONLY"), and ensure any module
exports/imports remain correct so tests continue to import
verify_vtpm_in_windows_vm; alternatively, if you truly intend to delete it,
remove all imports/usages (notably in tests/storage/cdi_clone/test_clone.py) and
delete the function instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 11e3ec8d-d6fa-4f0b-9a1f-4e1aa2980145
📒 Files selected for processing (2)
tests/storage/cdi_clone/test_clone.pytests/storage/utils.py
| def create_windows_vm_with_vtpm_validate_guest_agent_info( | ||
| dv, | ||
| namespace, | ||
| unprivileged_client, | ||
| windows_version, | ||
| admin_client=None, | ||
| ): | ||
| """ | ||
| Create Windows VM with vTPM using instance types and preferences. | ||
|
|
||
| Args: | ||
| dv: DataVolume to use for the VM | ||
| namespace: Namespace object | ||
| unprivileged_client: Client to use | ||
| windows_version: Windows version string ("11", "2k22", etc.) | ||
| admin_client: Optional admin client for vTPM validation | ||
| """ | ||
| from tests.storage.utils import validate_os_info_vmi_vs_windows_os, wait_for_windows_vm | ||
|
|
||
| # Map Windows version to preference name | ||
| preference_name = f"windows.{windows_version}" # e.g., "windows.11", "windows.2k22" | ||
|
|
||
| with VirtualMachineForTests( | ||
| name=f"vm-win-{windows_version}-vtpm", | ||
| namespace=namespace.name, | ||
| client=unprivileged_client, | ||
| os_flavor=OS_FLAVOR_WINDOWS, | ||
| vm_instance_type=VirtualMachineClusterInstancetype(name="u1.large", client=unprivileged_client), | ||
| vm_preference=VirtualMachineClusterPreference(name=preference_name, client=unprivileged_client), | ||
| data_volume_template={"metadata": dv.res["metadata"], "spec": dv.res["spec"]}, | ||
| ) as vm: | ||
| wait_for_windows_vm(vm=vm, version=windows_version, timeout=TIMEOUT_40MIN) | ||
| validate_os_info_vmi_vs_windows_os(vm=vm) | ||
|
|
||
| # Validate vTPM if admin_client provided | ||
| if admin_client: | ||
| from tests.storage.utils import verify_vtpm_in_windows_vm | ||
|
|
||
| verify_vtpm_in_windows_vm(vm=vm, admin_client=admin_client) |
There was a problem hiding this comment.
[HIGH] Helper function must not live in a test_*.py module — and its imports must not live inside its body.
Multiple guideline violations are stacking up here:
-
Helper in test file — As per coding guidelines: "conftest.py is for fixtures only. Helper functions, utility functions, and classes must NOT be defined in conftest.py or test_*.py; place them in dedicated utility modules instead." Move
create_windows_vm_with_vtpm_validate_guest_agent_infototests/storage/utils.py(right next to the siblingcreate_windows_vm_validate_guest_agent_info). -
Imports inside function body (Lines 89, 108) — As per coding guidelines: "Imports must always be at the top of the Python module. Do not import inside functions." Also,
validate_os_info_vmi_vs_windows_osoriginates fromutilities.sspandwait_for_windows_vmfromutilities.virt; importing them viatests.storage.utilsas a re-export surface is misleading. Import from the canonical modules. -
Optional vTPM validation defeats the PR's goal —
admin_client=Noneplus theif admin_client:gate makes the whole reason this helper exists (vTPM verification) conditional and invisible to the caller. The PR title is literally "Windows tests should all use vTPM". As per coding guidelines: "Function behavior MUST be controlled via explicit arguments, not hardcoded internally. No hidden side effects" and "Do not use defensive programming. Fail fast and don't hide bugs with fake defaults." Makeadmin_clienta required positional argument. -
Identity comparison for None — even if you keep the optional form, use
if admin_client is not None:. As per coding guidelines: "Use identity for None:if x is None:".
♻️ Proposed refactor (after moving to tests/storage/utils.py)
-def create_windows_vm_with_vtpm_validate_guest_agent_info(
- dv,
- namespace,
- unprivileged_client,
- windows_version,
- admin_client=None,
-):
- """
- Create Windows VM with vTPM using instance types and preferences.
-
- Args:
- dv: DataVolume to use for the VM
- namespace: Namespace object
- unprivileged_client: Client to use
- windows_version: Windows version string ("11", "2k22", etc.)
- admin_client: Optional admin client for vTPM validation
- """
- from tests.storage.utils import validate_os_info_vmi_vs_windows_os, wait_for_windows_vm
-
- # Map Windows version to preference name
- preference_name = f"windows.{windows_version}" # e.g., "windows.11", "windows.2k22"
-
- with VirtualMachineForTests(
- name=f"vm-win-{windows_version}-vtpm",
- namespace=namespace.name,
- client=unprivileged_client,
- os_flavor=OS_FLAVOR_WINDOWS,
- vm_instance_type=VirtualMachineClusterInstancetype(name="u1.large", client=unprivileged_client),
- vm_preference=VirtualMachineClusterPreference(name=preference_name, client=unprivileged_client),
- data_volume_template={"metadata": dv.res["metadata"], "spec": dv.res["spec"]},
- ) as vm:
- wait_for_windows_vm(vm=vm, version=windows_version, timeout=TIMEOUT_40MIN)
- validate_os_info_vmi_vs_windows_os(vm=vm)
-
- # Validate vTPM if admin_client provided
- if admin_client:
- from tests.storage.utils import verify_vtpm_in_windows_vm
-
- verify_vtpm_in_windows_vm(vm=vm, admin_client=admin_client)
+# In tests/storage/utils.py, with top-level imports:
+# from utilities.ssp import validate_os_info_vmi_vs_windows_os
+# from utilities.virt import VirtualMachineForTests, wait_for_windows_vm
+# from ocp_resources.virtual_machine_cluster_instancetype import VirtualMachineClusterInstancetype
+# from ocp_resources.virtual_machine_cluster_preference import VirtualMachineClusterPreference
+
+def create_windows_vm_with_vtpm_validate_guest_agent_info(
+ dv,
+ namespace,
+ unprivileged_client,
+ admin_client,
+ windows_version,
+):
+ """Create a Windows VM with vTPM via instancetype/preference and validate guest agent + vTPM.
+
+ Args:
+ dv: Cloned DataVolume whose metadata/spec seed the VM's dataVolumeTemplate.
+ namespace: Namespace where the VM is created.
+ unprivileged_client: Client used to create the VM.
+ admin_client: Admin client required for privileged VM XML access during vTPM verification.
+ windows_version: Windows version suffix (e.g. "11", "2k22") used to pick the preference.
+ """
+ with VirtualMachineForTests(
+ name=f"vm-win-{windows_version}-vtpm",
+ namespace=namespace.name,
+ client=unprivileged_client,
+ os_flavor=OS_FLAVOR_WINDOWS,
+ vm_instance_type=VirtualMachineClusterInstancetype(name="u1.large", client=unprivileged_client),
+ vm_preference=VirtualMachineClusterPreference(
+ name=f"windows.{windows_version}", client=unprivileged_client
+ ),
+ data_volume_template={"metadata": dv.res["metadata"], "spec": dv.res["spec"]},
+ ) as vm:
+ wait_for_windows_vm(vm=vm, version=windows_version, timeout=TIMEOUT_40MIN)
+ validate_os_info_vmi_vs_windows_os(vm=vm)
+ verify_vtpm_in_windows_vm(vm=vm, admin_client=admin_client)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/storage/cdi_clone/test_clone.py` around lines 72 - 110, Move the helper
create_windows_vm_with_vtpm_validate_guest_agent_info out of the test module
into tests/storage/utils.py (next to
create_windows_vm_validate_guest_agent_info), make admin_client a required
positional argument (remove the =None default) so vTPM validation always runs,
relocate the imports to the top of the new module (import
validate_os_info_vmi_vs_windows_os from utilities.ssp, wait_for_windows_vm from
utilities.virt, and verify_vtpm_in_windows_vm from tests.storage.utils or the
correct canonical module), and remove the runtime None check (do not gate
verify_vtpm_in_windows_vm with if admin_client; call it directly). Ensure any
remaining None comparisons use identity (if x is None) when applicable.
| vm_preference=VirtualMachineClusterPreference(name=preference_name, client=unprivileged_client), | ||
| data_volume_template={"metadata": dv.res["metadata"], "spec": dv.res["spec"]}, | ||
| ) as vm: | ||
| wait_for_windows_vm(vm=vm, version=windows_version, timeout=TIMEOUT_40MIN) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether wait_for_windows_vm starts the VM, and cross-check the sibling pattern.
ast-grep --pattern 'def wait_for_windows_vm($$$):
$$$'
echo "---"
rg -nP --type=py -C2 '\brunning_vm\s*\(|\bvm\.start\s*\(' utilities/virt.py | head -n 80Repository: RedHatQE/openshift-virtualization-tests
Length of output: 1959
🏁 Script executed:
# Get the context around line 103 in test_clone.py
cat -n tests/storage/cdi_clone/test_clone.py | sed -n '95,115p'Repository: RedHatQE/openshift-virtualization-tests
Length of output: 1140
🏁 Script executed:
# Get the sibling pattern from tests/storage/utils.py around line 375
cat -n tests/storage/utils.py | sed -n '370,385p'Repository: RedHatQE/openshift-virtualization-tests
Length of output: 781
🏁 Script executed:
# Check the full wait_for_windows_vm function to confirm it doesn't start the VM
cat -n utilities/virt.py | sed -n '1693,1710p'Repository: RedHatQE/openshift-virtualization-tests
Length of output: 739
Add running_vm(vm=vm) before wait_for_windows_vm(...).
VirtualMachineForTests creates the VM with run_strategy=VirtualMachine.RunStrategy.HALTED, so the context manager at line 102 creates a stopped VM. The wait_for_windows_vm() function does not start the VM—it immediately attempts to execute vm.ssh_exec.run_command() (utilities/virt.py:1703), which requires SSH connectivity and therefore a Running VM. Without the VM in Running state, the SSH command will fail.
The sibling helper create_windows19_vm (tests/storage/utils.py:375) explicitly calls running_vm(vm=vm) before yielding, following the same pattern. Add the same call here before line 103:
Suggested change
) as vm:
running_vm(vm=vm)
wait_for_windows_vm(vm=vm, version=windows_version, timeout=TIMEOUT_40MIN)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/storage/cdi_clone/test_clone.py` at line 103, The test creates the VM
in a halted state (VirtualMachine.RunStrategy.HALTED) so
wait_for_windows_vm(vm=vm, ...) fails because it immediately runs SSH; call
running_vm(vm=vm) to transition the VM to Running before calling
wait_for_windows_vm. Insert a running_vm(vm=vm) invocation inside the context
manager just prior to wait_for_windows_vm (mirroring create_windows19_vm) so the
VM is started and SSH connectivity is available.
| windows_version: Windows version string ("11", "2k22", etc.) | ||
| admin_client: Optional admin client for vTPM validation | ||
| """ | ||
| from tests.storage.utils import validate_os_info_vmi_vs_windows_os, wait_for_windows_vm |
There was a problem hiding this comment.
Please move imports to the beginning of the file
| namespace=namespace.name, | ||
| client=unprivileged_client, | ||
| os_flavor=OS_FLAVOR_WINDOWS, | ||
| vm_instance_type=VirtualMachineClusterInstancetype(name="u1.large", client=unprivileged_client), |
There was a problem hiding this comment.
Please use U1_LARGE constant
| vm_console.expect(pattern=file_content, timeout=TIMEOUT_20SEC) | ||
|
|
||
|
|
||
| def verify_vtpm_in_windows_vm(vm, admin_client): # TODO: Remove - debug only |
There was a problem hiding this comment.
Please mark PR as work-in-progress
/wip
| with create_dv( | ||
| client=unprivileged_client, | ||
| source="pvc", | ||
| dv_name="dv-target-win11-vtpm", | ||
| namespace=data_volume_multi_storage_scope_function.namespace, | ||
| size=data_volume_multi_storage_scope_function.size, | ||
| source_pvc=data_volume_multi_storage_scope_function.name, | ||
| storage_class=data_volume_multi_storage_scope_function.storage_class, | ||
| ) as cdv: |
There was a problem hiding this comment.
So here, you created a DV, which is cloned from data_volume_multi_storage_scope_function
| create_windows_vm_with_vtpm_validate_guest_agent_info( | ||
| dv=cdv, | ||
| namespace=namespace, | ||
| unprivileged_client=unprivileged_client, | ||
| windows_version="11", | ||
| admin_client=admin_client, | ||
| ) |
There was a problem hiding this comment.
And here, the way this function is written right now, your VM will create a new DV, which is a clone of cdv.
| { | ||
| "dv_name": "dv-source-win11-vtpm", | ||
| "source": "http", | ||
| "image": f"{Images.Windows.DIR}/{Images.Windows.WIN11_IMG}", |
There was a problem hiding this comment.
It may be better to use this image - images in the 'registry' are better maintained. And WINDOWS_2022 is more preferable than Win11.
url=f"{get_test_artifact_server_url(schema='registry')}/{WINDOWS_2022[CONTAINER_DISK_IMAGE_PATH_STR]}", (see how it's used in another test)
Signed-off-by: Adam Cinko <acinko@redhat.com>
openshift-virtualization-qe-bot-4
changed the title
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
Signed-off-by: Adam Cinko <acinko@redhat.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4571 +/- ##
==========================================
+ Coverage 98.63% 98.66% +0.03%
==========================================
Files 25 25
Lines 2420 2477 +57
==========================================
+ Hits 2387 2444 +57
Misses 33 33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
openshift-virtualization-qe-bot-4
requested review from
RoniKishner,
dshchedr,
geetikakay,
rnetser and
vsibirsk
Signed-off-by: Adam Cinko <acinko@redhat.com>
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
7 participants
Short description:
PR merged to tier-2/3 that makes Windows test VMs in sig-storage use vTPM
(preferably through instancetypes, which should be the right and easy way to achieve this)
More details:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
Co-Authored: Claude Code
jira-ticket:
Summary by CodeRabbit